feat: add interlinear PDF export pipeline#134
feat: add interlinear PDF export pipeline#134delgado-jacob wants to merge 1 commit intoglobalbibletools:mainfrom
Conversation
arrocke
left a comment
arrocke
left a comment
There was a problem hiding this comment.
Wow thanks for the contribution! There is a lot of good stuff in here, but a PR this size is very difficult to review. I'm going to treat this like a prototype to work through product/design decisions together. Then let's chop up the PR up into smaller PRs for easier technical review. There will be a lot to work through and this will make it less overwhelming to review and faster to get changes shipped for testing.
In the future, I encourage you to seek clarity on vague tickets before starting work and open PRs with smaller changes. We have a feature flag system to be able to do this with incomplete features.
We can use this PR to discuss the bigger picture of this feature, so I will leave some high level feedback here to start that discussion. From there, here is a way we can break this work up into smaller PRs for easier review:
- Introduce localstack: I'm thrilled you are introducing this. There are a number of other places where we could take advantage of this. This PR could simply introduce localstack and its initialization scripts.
- Basic export job infrastructure: Set up the UI to trigger the new job and report it's status. At this stage the job is a noop. This new UI can be behind a feature flag.
- Upload logic: Add the logic to upload an empty PDF in the job.
- PDF generation: Add the MVP PDF generation logic to the job. This will be the meat of the work, but we'll be able to focus on just that logic now that all of the infrastructure is taken care of.
Please don't be discouraged by this feedback. My goal is to clarify how we can best work together. I'm grateful for the work you've put in thus far to develop this feature. Please let me know if you have any questions.
| await enqueueJob(SNAPSHOT_JOB_TYPES.CREATE_SNAPSHOT_INTERLINEAR_PDF, { | ||
| languageId: snapshot.languageId, | ||
| languageCode: language.code, | ||
| snapshotId: snapshot.id, | ||
| }); |
There was a problem hiding this comment.
The snapshot system is meant for backup/restore purposes, not to export for general consumption. We can remove PDF generation from the snapshot module
| @@ -0,0 +1,36 @@ | |||
| create type export_request_status as enum ('PENDING', 'IN_PROGRESS', 'COMPLETE', 'FAILED'); | |||
|
|
|||
| create table export_request ( | |||
There was a problem hiding this comment.
I'd prefer to track all of this in the payload and data columns of the job table. Both of those columns can hold arbitrary JSON. Jobs are ephemeral so I find referential integrity of job data to be less important and having a single mechanism for tracking job progress is useful. Do you have any concerns with this approach?
There was a problem hiding this comment.
Sounds good, no concerns.
| import { defineConfig } from "vitest/config"; | ||
| import tsconfigPaths from "vite-tsconfig-paths"; | ||
|
|
||
| export default defineConfig({ |
There was a problem hiding this comment.
I'm not sure we want to require localstack to be running for integration tests. I think we can get away with stubbing the s3 api and asserting it is called with the right data
| @@ -0,0 +1,57 @@ | |||
| services: | |||
There was a problem hiding this comment.
Is there a reason to break this out into it's own file? Our compose.yaml is for locale development only since everything is deployed separately in production environments
There was a problem hiding this comment.
No, just precaution to avoid messing with any other workflows. I can consolidate it.
| <InterlinearExportPanel | ||
| languageCode={languageSettings.code} | ||
| books={books} | ||
| /> |
There was a problem hiding this comment.
I'd like to move this to its own page in the admin language view. The export module would own that view. The settings page is reserved for configuration on the language
| export interface PdfGeneratorOptions { | ||
| pageSize?: "letter" | "a4"; | ||
| layout: "standard" | "parallel"; | ||
| direction?: "ltr" | "rtl"; | ||
| header?: { title?: string; subtitle?: string }; | ||
| footer?: { generatedAt?: Date; pageOffset?: number; pageTotal?: number }; | ||
| sourceScript?: "hebrew" | "greek"; | ||
| } |
There was a problem hiding this comment.
For this first version, I'd like to simplify this to a single button for the user. That button converts to a loading UI while the job is in progress and then reports when the job is done.
For the PDF, let's use your standard layout and export all books by default even if there are no glosses for that book yet. This is a true MVP of this feature and we can expand options later.
Makes sense, I'll work on splitting things up. Are you ok with stacked branches or would you prefer a single branch at a time in PR?
Not at all. My main objective was to start getting familiar with the platform and prototype something for discussion/iteration. I realize it's a large change set and have no issues breaking it up. |
|
Stacked branches are completely fine. Since we squash a PR into a single commit, the branch history can be a little messy and it will all consolidate to a single commit in the main branch. Open stacked PRs whenever you'd like, just leave them in draft state until you have something you want to merge. PRs are the best place to ask questions about the changes you are making so opening them early and mentioning me with specific questions will speed up the review time later rather than addressing all of that at the end. I'll try to be prompt in responding. |
2 participants
closes: #109
Feature
For Translators:
For Snapshots:
Architecture
New export Module (src/modules/export/)
Background Jobs
Database Changes
Key Design Decisions
Testing